-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add network configuration check and report to ros2doctor #319
Conversation
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
waiting for rosdistro PR approval ros/rosdistro#22071
Signed-off-by: claireyywang <clairewang@openrobotics.org>
…os2cli into claire/ros2doctor-network Signed-off-by: claireyywang <clairewang@openrobotics.org>
658e710
to
3373522
Compare
ros2doctor/package.xml
Outdated
@@ -9,7 +9,7 @@ | |||
|
|||
<depend>ros2cli</depend> | |||
|
|||
<exec_depend>python3-numpy</exec_depend> | |||
<exec_depend>ifcfg-pip</exec_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a Debian release of this package? Packages released into a ROS distro (like the packages in this repo) can't use packages from PyPi
because .deb
s can only depend on other Debian packages.
If there isn't a Debian release, an alternative is to release our own deb
, which means creating a vendor package for ifcfg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vendor package for ifcfg
at https://github.com/ros2/ifcfg_vendor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to change this dependency to ifcfg_vendor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I also need to change to ifcfg_vendor on rosdistro rosdep key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I also need to change to ifcfg_vendor on rosdistro rosdep key?
I don't think so. ifcfg-pip
is the right name for the rosdistro key (the key isn't needed for this PR since there's a vendor package). The package.xml should be changed to depend on the new package ifcfg_vendor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly nitpicks
ros2doctor/package.xml
Outdated
@@ -9,7 +9,7 @@ | |||
|
|||
<depend>ros2cli</depend> | |||
|
|||
<exec_depend>python3-numpy</exec_depend> | |||
<exec_depend>ifcfg-pip</exec_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to change this dependency to ifcfg_vendor
.
ros2doctor/ros2doctor/api/format.py
Outdated
def print_term(k, v): | ||
"""Print term only if it exists.""" | ||
if v: | ||
# TODO: 20 padding needs to be dynamically set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, add parenthesis with your github username. TODO(claireyywang)
. The name in the parenthesis is the person who can answer questions about what needs to be done. Most of the time it's the person who created the comment.
ros2doctor/ros2doctor/api/network.py
Outdated
|
||
def _is_unix_like_platform(): | ||
"""Return True if conforms to UNIX/POSIX-style APIs.""" | ||
return platform.system() in ['Linux', 'FreeBSD', 'Darwin'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend 'posix' == os.name
instead
ros2doctor/ros2doctor/api/network.py
Outdated
return platform.system() in ['Linux', 'FreeBSD', 'Darwin'] | ||
|
||
|
||
def check_sys_ips(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function name doesn't seem very descriptive. It's a little more confusing in the context of ros2doctor
since it's not one of the ros2doctor.checks
. I don't have an alternative though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing to check_network_config_helper
ros2doctor/ros2doctor/api/network.py
Outdated
if 'LOOPBACK' in flags: | ||
has_loopback = True | ||
else: | ||
has_others = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommend has_non_loopback
instead
ros2doctor/ros2doctor/api/network.py
Outdated
sys.stderr.write('WARNING: Only loopback IP address is found.\n') | ||
if not has_multicast: | ||
sys.stderr.write('WARNING: No multicast IP address is found.\n') | ||
if has_loopback and has_others and has_multicast: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, can eliminate result
with return has_loopback and has_others and has_multicast
print('build : ', platform.python_build()) | ||
print_term('version', platform.python_version()) | ||
print_term('compiler', platform.python_compiler()) | ||
print_term('build', platform.python_build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an improvement. I still think it would be good if the reports
just returned information and the command was responsible for outputting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about making that change as a feature in the next PR. Like you suggested last time, I'd like to add arguments like --report_all
or --report_failed
to let users control what they would like to see based on the check results.
Since adding a vendor package for |
|
Signed-off-by: claireyywang <clairewang@openrobotics.org>
386f0b1
to
2389097
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Before merging this, please also create a PR adding ifcfg_vendor
to ros2.repos
(like this PR). This and that will need to be merged at the same time to avoid breaking CI.
Most changes happen in
ros2doctor/api/network.py
. Network configuration is checked and printed usingifcfg
python-pip package.ros2doctor/api/format.py
is created to format the report. Current padding is 20 chars, but will be changed in the future to dynamically allocate padding based on string length. I have also modified print statements inros2doctor/api/platform.py
to reflect new report format.